-
Notifications
You must be signed in to change notification settings - Fork 386
Conversation
3c99936
to
e33b7f0
Compare
e33b7f0
to
839299f
Compare
I forgot to add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Have a question about some deleted tests in client-daemonset.bats
. And as you mentioned, the CHANGELOG entry for meshgateway removal
.circleci/config.yml
Outdated
@@ -154,7 +154,7 @@ jobs: | |||
-kubecontext="kind-dc1" \ | |||
-secondary-kubecontext="kind-dc2" \ | |||
-debug-directory="$TEST_RESULTS/debug" \ | |||
-consul-k8s-image=hashicorpdev/consul-k8s:latest | |||
-consul-k8s-image=ghcr.io/lkysow/consul-k8s-dev:jan11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a reminder to delete this pre-merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Will hit approve once the changelog has the addition for meshgateway!! Great job Luke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
I've added this removal now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! My only comment that I think is currently blocking is about handling a case when .Values.connectInject.centralConfig
is set to true
. This could be the case if you are super explicit in your helm config and still set it, even though the default was true.
1c0b30e
to
059f06e
Compare
Changes proposed in this PR:
connectInject.centralConfig.defaultProtocol
. Use of this setting will now cause an error onhelm upgrade
.connectInject.centralConfig.proxyDefaults
. Use of this setting will now cause an error onhelm upgrade
.connectInject.centralConfig.enabled
tofalse
. Its default was previouslytrue
but we did allow you to set it tofalse
if you weren't using connect inject. Now you can't set it at all and we default it to true. Consul made the defaulttrue
in 1.9.0 and removing the ability to set it tofalse
in the Helm chart lets us fully remove theconnectInject.centralConfig
block. Users can still set it to false viaextraConfig
.meshGateway.globalMode
. Use of this setting will now cause an error onhelm upgrade
.How I've tested this PR:
connect-inject-deployment.bats
because that's where thefail
commands are.centralConfig.enabled
tofalse
and kept the tests that ensure it's set to true by default.How I expect reviewers to test this PR:
Checklist: